a5: batch sync_to_sm every K=16 local advances - #1575
Open
yanghaoran29 wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe PR adds an OCCUPY-based AICPU topology fallback, throttles ring scheduler updates to shared memory, and introduces A5-specific benchmark mappings with a single-example benchmark wrapper. ChangesAICPU topology fallback
Ring scheduler synchronization
A5 benchmark selection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Probe as probe_aicpu_topology_uncached
participant CPU_TOPO
participant OCCUPY as OCCUPY bitmap
Probe->>CPU_TOPO: query_cpu_topo
CPU_TOPO-->>Probe: entries or failure
Probe->>OCCUPY: intersect reported CPU IDs
OCCUPY-->>Probe: usable IDs or empty result
Probe->>OCCUPY: synthesize pool when needed
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/pto_scheduler.h`:
- Line 475: Update advance_ring_pointers() and its sync_to_sm() ordering so
ring->fc.last_task_alive is stored with last_task_alive once PUBLISH_INTERVAL_K
slots have been consumed, avoiding the extra-call throttle; when reuse resets
last_published_to_sm to 0, publish immediately after the local lifetime advance.
- Around line 467-478: Update the scheduler’s terminal, idle, and drained-work
paths around advance_ring_pointers() to force-publish the current
last_task_alive watermark, including sub-interval advances of 1–15 steps.
Preserve batched sync_to_sm() behavior for ongoing work, and add coverage for 1,
15, 16, and 17 advances to verify final publication and threshold batching.
In `@tools/benchmark_a5_case1.sh`:
- Around line 17-21: Update the option handling case for -d|--device and
-n|--rounds in the argument parser to validate that a following value exists
before reading $2 or executing shift 2. When either option is missing its value,
emit the script’s usage error and terminate consistently; preserve the existing
assignments and shifts for valid values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f3f19174-72e0-4e19-bda6-6ddebea50fd0
📒 Files selected for processing (4)
src/a5/platform/onboard/host/aicpu_topology_probe.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/pto_scheduler.htools/benchmark_a5_case1.shtools/benchmark_rounds.sh
Publish last_task_alive every 16 local advances while work remains, and force the drained tail so reclamation cannot stall on a partial batch. Reset the publication shadow on ring reuse and cover threshold and drain behavior in A5 unit tests.
yanghaoran29
force-pushed
the
exp/pr906-F3
branch
from
July 29, 2026 08:50
511a165 to
526eb8b
Compare
yanghaoran29
changed the title
Contributor
Author
A5 performance re-testRe-tested
Test details:
On this host, both CPU topology query paths return 65534, so unmodified main and the current PR cannot pass the AICPU affinity gate. For measurement only, the same OCCUPY-only topology fallback was applied to temporary baseline and PR worktrees. It was therefore a shared control variable; the only A/B code difference was the K=16 scheduler publication change. The fallback was not added back to this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RingSchedState::last_task_aliveto shared memory every 16 local advances while work remains.Validation
Extract from PR #906.